Skip to content

frontend: Dialog: move useCallback before early return to fix rules-of-hooks#5208

Merged
illume merged 1 commit intokubernetes-sigs:mainfrom
jedbillyb:fix/dialog-rules-of-hooks
Apr 25, 2026
Merged

frontend: Dialog: move useCallback before early return to fix rules-of-hooks#5208
illume merged 1 commit intokubernetes-sigs:mainfrom
jedbillyb:fix/dialog-rules-of-hooks

Conversation

@jedbillyb
Copy link
Copy Markdown
Contributor

@jedbillyb jedbillyb commented Apr 23, 2026

Fixes #5186

Moves React.useCallback in DialogTitle to before the early return
guard clause, satisfying rules-of-hooks. Also adds focusTitle to the
dependency array which was previously missing.

Steps to test:

  • Run npx eslint src/components/common/Dialog.tsx - no rules-of-hooks warnings

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 23, 2026
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 23, 2026
@kahirokunn
Copy link
Copy Markdown
Member

kahirokunn commented Apr 23, 2026

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2026
@illume illume requested a review from Copilot April 23, 2026 13:27
Copilot AI reviewed Apr 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an ESLint react-hooks/rules-of-hooks violation in DialogTitle by ensuring useCallback is always invoked before any early returns, and updates the hook dependencies to match referenced props.

Changes:

  • Move the focusedRef React.useCallback above the guard-clause early return in DialogTitle.
  • Add focusTitle to the useCallback dependency array (previously missing).

Comment thread frontend/src/components/common/Dialog.tsx Outdated
illume
illume requested changes Apr 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, another PR was merged and there's now a git conflict.

Are you able to fix the git conflict?

illume

This comment was marked as duplicate.

Sign in to view

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2026
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Apr 23, 2026

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 23, 2026
@jedbillyb
Copy link
Copy Markdown
Contributor Author

jedbillyb commented Apr 23, 2026

@illume fixed!

skoeva
skoeva requested changes Apr 24, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see this comment

@jedbillyb jedbillyb force-pushed the fix/dialog-rules-of-hooks branch from 19c7021 to b596018 Compare April 25, 2026 04:56
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 25, 2026
@jedbillyb
Copy link
Copy Markdown
Contributor Author

jedbillyb commented Apr 25, 2026

@illume @skoeva updates are in:

  • moved useCallback before early return (rules-of-hooks)
  • included focusTitle in dependency array
  • resolved rebase/conflict and cleaned history
  • all CI checks are now green

Could you please re-review?

illume
illume requested changes Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these changes.

Can you please have a look at the git commits to see if they meet the contribution guidelines? We use a Linux kernel style of git commits. See the contributing guide for general context, and please see previous git commits with git log for examples.

Commit guidelines
  • Use atomic commits focused on a single change.
  • Use the title format <area>: <description of changes>.
  • Keep the title and body lines under 72 characters.
  • Explain the intention and why the change is needed.
  • Make commit titles meaningful and describe what changed.
  • Do not add code that a later commit rewrites; squash or reorder commits instead.
  • Do not include Fixes #NN in commit messages.

Good examples:

  • frontend: HomeButton: Fix so it navigates to home
  • backend: config: Add enable-dynamic-clusters flag

Can you please address the open review comments?

@jedbillyb jedbillyb force-pushed the fix/dialog-rules-of-hooks branch from b596018 to 943aff7 Compare April 25, 2026 12:03
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 25, 2026
@jedbillyb jedbillyb force-pushed the fix/dialog-rules-of-hooks branch from 943aff7 to 1a3555e Compare April 25, 2026 12:14
@jedbillyb
frontend: Dialog: move useCallback before early return
8c74ef6 
The focusedRef useCallback in DialogTitle was called after a guard
clause early return, violating rules-of-hooks. Move it before the
early return so the hook is always called unconditionally.

Also type the ref callback parameter as HTMLElement | null and
simplify the null check, and add focusTitle to the dependency array
which was previously missing.
@jedbillyb jedbillyb force-pushed the fix/dialog-rules-of-hooks branch from 1a3555e to 8c74ef6 Compare April 25, 2026 12:17
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 25, 2026
illume
illume approved these changes Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 🎉

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Apr 25, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: illume, jedbillyb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2026
@illume illume merged commit 9a95520 into kubernetes-sigs:main Apr 25, 2026
11 of 12 checks passed
@jedbillyb jedbillyb deleted the fix/dialog-rules-of-hooks branch April 25, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@skoeva skoeva skoeva requested changes

@illume illume illume approved these changes

@kahirokunn kahirokunn Awaiting requested review from kahirokunn

@sniok sniok Awaiting requested review from sniok

Assignees

@kahirokunn kahirokunn

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

6 participants

@jedbillyb @kahirokunn @k8s-ci-robot @illume @skoeva